🚀 Add new MCU support (C6 and C5) to the ESP-IDF V5 builds #5048
Conversation
due to Arduino 3.0.1, a few updated libraries are required: * Tasmota Platform - official platfomio lacks arduino support for C6 * FastLED (latest + C6 build patches) * AsyncTCP (latest + C6 build patches) * AsyncWebServer (latest + C6 build patches) * NeoPixelBus (lastest)
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds ESP32-C5 and ESP32-C6 build environments and PlatformIO config; expands many CONFIG_IDF_TARGET_* guards across sources so C5/C6 (and P4) are treated like C3 for I2S/touch/PSRAM/DMX/pin-safety and other platform-specific logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
platformio.ini
Outdated
| ;nodemcuv2 | ||
| ;esp8266_2m | ||
| ;esp01_1m_full | ||
| ;nodemcuv2_160 | ||
| ;esp8266_2m_160 | ||
| ;esp01_1m_full_160 | ||
| ;nodemcuv2_compat | ||
| ;esp8266_2m_compat | ||
| ;esp01_1m_full_compat | ||
| ;esp32dev | ||
| ;esp32dev_debug | ||
| ;esp32_eth | ||
| ;esp32_wrover | ||
| ; lolin_s2_mini ;; TODO: disabled NeoEsp32RmtMethodIsr | ||
| esp32c3dev | ||
| ;esp32c3dev |
There was a problem hiding this comment.
TODO: revert before merge, here to save CI build time
|
@softhack007 I've started trying to cherry-pick your changes over, quite a few don't apply due to the changes with MM, but I'll try and resolve the merge conflicts for those that are simple |
* more debug output * added my own fork of FastLED ( looks like more bugs to solve ....)
@netmindz thanks, I'll try to help once I'm finished with some open ends in WLED-MM 😁 december |
|
Possibly also worth looking at #4626 to see if there are any changes there that are helpful |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
also disabled some core debug flags
this was a temporary hack needed one year ago; FastLED should be compatible with -C6 now.
trying to reduce compile errors in bus_wrapper.h ... plus some preparation for P4 support
similar to C3
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
wled00/FX_fcn.cpp (1)
1194-1211:⚠️ Potential issue | 🟠 MajorFix single‑I2S DMA memory estimation for ESP32/S2.
usesI2Sstill assumes single‑I2S is the first digital bus (and doesn’t account for S2 single‑I2S), which underestimatesmaxI2Sonce multiple digital buses are configured. With single‑I2S now mapped to the last bus, the DMA memory estimate should key off that bus index to avoid allowing oversized configs through the MAX_LED_MEMORY check.Based on learnings: In WLED PR `#5267`, the single‑I2S bus moved to the last bus (ESP32 bus 8, ESP32‑S2 bus 4), so memory calculations must account for that specific bus index.🔧 Suggested adjustment (align single‑I2S bus index)
- `#if` defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S3) - const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1)); - `#elif` defined(CONFIG_IDF_TARGET_ESP32S2) - const bool usesI2S = (useParallelI2S && digitalCount <= 8); + `#if` defined(CONFIG_IDF_TARGET_ESP32) + const unsigned singleI2SBus = 9; // bus 8 (last) when single I2S + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == singleI2SBus); + `#elif` defined(CONFIG_IDF_TARGET_ESP32S3) + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1); + `#elif` defined(CONFIG_IDF_TARGET_ESP32S2) + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 5); // bus 4 (last) when single I2Swled00/wled.h (2)
158-165:⚠️ Potential issue | 🟡 MinorDMX error message should mention C61.
The guard includes ESP32‑C61, but the compile‑time error text doesn’t.
✏️ Suggested message update
- `#error` "DMX output is not supported on ESP32-C5/C6/P4 (esp_dmx library excluded)" + `#error` "DMX output is not supported on ESP32-C5/C6/C61/P4 (esp_dmx library excluded)"- `#error` "DMX output is not supported on ESP32-C5/C6/P4 (esp_dmx library excluded)" + `#error` "DMX output is not supported on ESP32-C5/C6/C61/P4 (esp_dmx library excluded)"Also applies to: 480-483
334-344:⚠️ Potential issue | 🔴 CriticalGuard RX/TX usage for non‑classic ESP32 targets.
The current guard logic has a flaw: for non-classic ESP32s (S2/S3/C3/C5/C6/C61/P4), the condition
!defined(CONFIG_IDF_TARGET_ESP32)short-circuits the OR to TRUE, bypassing the RX/TX existence check. Since some Arduino-ESP32 board variants don't define these macros, this will cause compilation failures. Require explicit RX/TX presence for all ESP32 targets:-#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX))) +#if defined(ARDUINO_ARCH_ESP32) && defined(RX) && defined(TX) constexpr uint8_t hardwareRX = RX; constexpr uint8_t hardwareTX = TX; `#else` // use defaults for RX/TX constexpr uint8_t hardwareRX = 3; constexpr uint8_t hardwareTX = 1; `#endif`
🤖 Fix all issues with AI agents
In `@wled00/wled_boards.h`:
- Around line 130-136: The comment marking RTC memory as "TBC" is misleading
because util.cpp (around util.cpp:701) already uses MALLOC_CAP_RTCRAM for
conditional allocation; update wled_boards.h by removing the "TBC" text and
either (a) leave `#define` WLED_HAVE_RTC_MEMORY_HEAP 1 as a confirmed capability
(matching how ESP32-C61 is documented) or (b) gate the definition behind the
proper IDF config macro (e.g., only define WLED_HAVE_RTC_MEMORY_HEAP when
CONFIG_IDF_TARGET_ESP32P4 or the appropriate ESP-IDF capability macro is
present); ensure the comment style matches other board entries and that
WLED_HAVE_RTC_MEMORY_HEAP is treated consistently with util.cpp usage.
🧹 Nitpick comments (2)
wled00/ota_update.cpp (1)
18-23: Keep bootloader offset single‑sourced.Now that offsets vary by target, consider wiring the OTA context to
BOOTLOADER_OFFSETto prevent future mismatches.Suggested follow-up
- const uint32_t bootloaderOffset = 0x1000; + const uint32_t bootloaderOffset = BOOTLOADER_OFFSET;platformio.ini (1)
392-420: Pin the NeoPixelBus C5 fork to a commit hash instead of a branch ref.
Branch pointers can move or be deleted, breaking reproducible builds. Use a commit hash or tag for CI artifacts. This aligns with the pattern used elsewhere in the file (e.g.,GifDecoder.git#bc3af189b6b1e06946569f6b4287f0b79a860f8e).
* remove last scan results when changing band mode * set band mode before starting a new scan
allow max Tx power = 21 dBm if framework supports it. If not supported, silently clamp to 19.5 dBm.
according to espressif docs, this is "maximum TX power" not "TX power"
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
* shortened another mega-ifdef * added missing CONFIG_IDF_TARGET_ESP32C61
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
usermods/audioreactive/audio_reactive.cpp (1)
1207-1232:⚠️ Potential issue | 🔴 CriticalAdd ESP32-C6 to hardware capability guards.
Both guards need C6 added:
- Line 1207: C6 doesn't support PDM microphones
- Line 1223: C6 doesn't support ADC over I2S (only classic ESP32 does)
Proposed fix
- `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) + `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) case 5: // ... PDM case `#endif` // ... - `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32S3) + `#if` !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32C5) && !defined(CONFIG_IDF_TARGET_ESP32C6) && !defined(CONFIG_IDF_TARGET_ESP32S3) // ADC over I2S is only possible on "classic" ESP32 case 0:wled00/FX_fcn.cpp (1)
1194-1211:⚠️ Potential issue | 🟠 MajorFix single‑I2S memory accounting to match bus placement.
WhenuseParallelI2Sis false, this still assumes the first digital bus uses I2S (digitalCount == 1). With single‑I2S moved to the last bus,maxI2Scan be computed from the wrong bus and under‑estimate DMA size, letting configs slip past theMAX_LED_MEMORYcheck. If S3 uses a different index, adjust accordingly.🔧 Proposed fix (align single‑I2S to the last bus)
- const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1)); + const bool usesI2S = useParallelI2S + ? (digitalCount <= 8) + : (digitalCount == 9); // single I2S lives on last bus (digitalCount=9) @@ - const bool usesI2S = (useParallelI2S && digitalCount <= 8); + const bool usesI2S = useParallelI2S + ? (digitalCount <= 8) + : (digitalCount == 5); // single I2S lives on last bus (digitalCount=5)Based on learnings: In WLED PR
#5267, the bus allocation strategy for single I2S mode was changed: ESP32 now uses single I2S on bus 8 (the last bus, digitalCount=9) instead of bus 0; ESP32-S2 uses single I2S on bus 4 (digitalCount=5). Memory calculation logic must account for which specific bus uses I2S hardware, not just check digitalCount <= 8.wled00/wled.h (2)
158-165:⚠️ Potential issue | 🟡 MinorFix DMX guard indentation and clarify the C61 error text.
Indent nested preprocessor directives by two spaces and include C61 in the
#errormessage to match the guard list. As per coding guidelines: Use 2-space indentation for non-generated C++ header files (.h).🛠️ Proposed fix
`#ifdef` WLED_ENABLE_DMX - `#if` defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) - `#error` "DMX output is not supported on ESP32-C5/C6/P4 (esp_dmx library excluded)" - `#elif` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3)|| defined(CONFIG_IDF_TARGET_ESP32S2) - `#include` "src/dependencies/dmx/ESPDMX.h" - `#else` //ESP32 - `#include` "src/dependencies/dmx/SparkFunDMX.h" - `#endif` + `#if` defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) + `#error` "DMX output is not supported on ESP32-C5/C6/C61/P4 (esp_dmx library excluded)" + `#elif` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3)|| defined(CONFIG_IDF_TARGET_ESP32S2) + `#include` "src/dependencies/dmx/ESPDMX.h" + `#else` //ESP32 + `#include` "src/dependencies/dmx/SparkFunDMX.h" + `#endif` `#endif` @@ `#ifdef` WLED_ENABLE_DMX - `#if` defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) - `#error` "DMX output is not supported on ESP32-C5/C6/P4 (esp_dmx library excluded)" - `#elif` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32S2) - WLED_GLOBAL DMXESPSerial dmx; - `#else` //ESP32 - WLED_GLOBAL SparkFunDMX dmx; - `#endif` + `#if` defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32P4) + `#error` "DMX output is not supported on ESP32-C5/C6/C61/P4 (esp_dmx library excluded)" + `#elif` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32S2) + WLED_GLOBAL DMXESPSerial dmx; + `#else` //ESP32 + WLED_GLOBAL SparkFunDMX dmx; + `#endif` `#endif`Also applies to: 480-487
334-344:⚠️ Potential issue | 🟠 MajorFix RX/TX detection:
defined()cannot detect C++ constants.In Arduino-ESP32,
RX/TXare defined as C++ constants (static const uint8_t) inpins_arduino.h, not preprocessor macros. The#if defined(RX)check cannot detect them. This causes the condition to always use hardcoded defaults (3, 1) for non-classic ESP32 variants, ignoring their actual UART0 pins (e.g., C3: 20/21, C5: 12/11, C6: 17/16, S2/S3: 44/43). Either use runtime assignment or include the actual Arduino headers to access the constants properly.
🤖 Fix all issues with AI agents
In `@usermods/audioreactive/audio_reactive.cpp`:
- Around line 1800-1803: The analog microphone JSON serialization blocks (e.g.,
the creation of JsonObject amic via top.createNestedObject and assignments of
amic["pin"] and related analog-mic fields) are missing ESP32-C6 in their
exclusion preprocessor guards; update the conditional macros around these
sections (the guard at the block creating amic and the subsequent analog-mic
serialization blocks) to also exclude CONFIG_IDF_TARGET_ESP32C6 (or switch to
the positive CONFIG_IDF_TARGET_ESP32 guard to target classic ESP32 only) so the
analog mic fields are omitted for ESP32S2, ESP32C3, ESP32C5, ESP32S3 and ESP32C6
consistently.
In `@wled00/bus_manager.cpp`:
- Around line 553-560: The TODO comment contains typos and an incorrect target
name; update the comment near the ledc_set_duty_and_update call (and the
surrounding LEDC/LEDC.channel_group comments) to fix spelling ("impemented" ->
"implemented") and correct the target reference ("P5" -> "P4" or "ESP32P4" to
match the `#if` condition). Ensure the TODO clearly references the LEDC struct
behavior and dithering support for ESP32P4 and leaves the rest of the technical
notes unchanged.
In `@wled00/wled.h`:
- Around line 427-429: The extern declaration for the global useParallelI2S in
bus_manager.cpp must be guarded to match its definition in wled.h; wrap the
extern bool useParallelI2S declaration with the same conditional compile guards
used for the definition (i.e., only declare it when building for ESP32 with
WLED_HAS_PARALLEL_I2S) so non-parallel I2S targets do not get an unresolved
extern reference.
🧹 Nitpick comments (2)
usermods/audioreactive/audio_reactive.cpp (1)
1163-1232: Consider centralizing hardware capability guards.The PR objectives mention
wled_boards.hwas added to centralize capability flags. Rather than repeating long#if !defined(...)chains throughout this file, consider defining capability macros like:// In wled_boards.h `#if` defined(CONFIG_IDF_TARGET_ESP32) `#define` WLED_HAS_ANALOG_MIC 1 `#define` WLED_HAS_PDM_MIC 1 `#elif` defined(CONFIG_IDF_TARGET_ESP32S3) `#define` WLED_HAS_PDM_MIC 1 `#endif`Then guards become simple:
#ifdef WLED_HAS_ANALOG_MIC— cleaner, future-proof, and consistent across files.wled00/ws.cpp (1)
83-96: Optional: name the DDP header length constant.The two bounds checks use the
10literal; extracting it avoids magic numbers and keeps the checks in sync.♻️ Suggested update
constexpr uint8_t BINARY_PROTOCOL_E131 = P_E131; // = 0, untested! constexpr uint8_t BINARY_PROTOCOL_ARTNET = P_ARTNET; // = 1, untested! constexpr uint8_t BINARY_PROTOCOL_DDP = P_DDP; // = 2 +constexpr size_t DDP_HEADER_LEN = 10; // bytes (excluding protocol byte) ... - if (len < size_t(10 + offset)) return; // DDP header is 10 bytes (+1 protocol byte) + if (len < size_t(DDP_HEADER_LEN + offset)) return; // DDP header is 10 bytes (+1 protocol byte) ... - if (len < size_t(10 + offset + ddpDataLen)) return; // not enough data, prevent out of bounds read + if (len < size_t(DDP_HEADER_LEN + offset + ddpDataLen)) return; // not enough data, prevent out of bounds readBased on learnings: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit.
these variables are defined in wled.h, which is included already.
|
@coderabbitai please remember for future reviews: check that all global variables in .cpp files are well-defined (via .h file or via "extern"). Verify that declarations are neither duplicated, nor implicitly declared as "int". |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
usermods/audioreactive/audio_source.h (1)
25-33:⚠️ Potential issue | 🟡 MinorUpdate the unsupported-target message to match the expanded guard.
The guard now includes ESP32‑C61 (and other targets already in the list), but the message only mentions C2/C3, which is misleading during build failures/warnings.
🔧 Suggested tweak
- `#error` This audio reactive usermod does not support ESP32-C2 or ESP32-C3. + `#error` This audio reactive usermod does not support this target. ... - `#warning` This audio reactive usermod does not support ESP32-C2 and ESP32-C3. + `#warning` This audio reactive usermod does not support this target.wled00/data/settings_wifi.htm (1)
72-80:⚠️ Potential issue | 🟡 MinorGuard against missing channel values in scan results.
Some scans omit
channel, which would render “Ch undefined” in the UI.🛠️ Suggested adjustment
- option.textContent = `${networks[i].ssid} (${networks[i].rssi} dBm, Ch ${networks[i].channel})`; // [${networks[i].bssid.replaceAll(':','')}] + const ch = Number.isFinite(networks[i].channel) ? `, Ch ${networks[i].channel}` : ""; + option.textContent = `${networks[i].ssid} (${networks[i].rssi} dBm${ch})`; // [${networks[i].bssid.replaceAll(':','')}]wled00/FX_fcn.cpp (1)
1163-1184:⚠️ Potential issue | 🟠 MajorFix I2S DMA buffer estimation for single‑I2S mode.
When
useParallelI2Sis false,usesI2Sonly triggers on the first digital bus. The current bus allocation places single‑I2S on the last digital bus, somaxI2Scan be underestimated and the MAX_LED_MEMORY check can pass when it shouldn’t. Track the total digital bus count and use that to detect the last bus.🐛 Suggested fix
- digitalCount = 0; + const unsigned totalDigitalBuses = digitalCount; + digitalCount = 0;- const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1)); + const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == totalDigitalBuses));- const bool usesI2S = (useParallelI2S && digitalCount <= 8); + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == totalDigitalBuses);Based on learnings: In WLED, both parallel I2S and single I2S modes require additional DMA buffers (4× the pixel buffer size). The memory calculation must account for which specific buses use I2S hardware, not just check digitalCount <= 8, because single I2S mode places the I2S bus at the END (ESP32 bus 8 with digitalCount=9, S2 bus 4 with digitalCount=5).
Also applies to: 1194-1209
🤖 Fix all issues with AI agents
In `@wled00/data/settings_wifi.htm`:
- Around line 253-258: You changed the WiFi UI (wled00/data/settings_wifi.htm —
elements with id "bm" and "tx"), so regenerate the embedded UI headers by
running the project's UI build step (npm run build) to update the generated
headers used in firmware builds; after running the build verify the generated
header files changed and include them in the PR so the firmware build stays in
sync with settings_wifi.htm.
🧹 Nitpick comments (2)
wled00/bus_wrapper.h (1)
509-513: Track the TODO to simplify the long guard.
Happy to help refactor this into a helper macro or wrapper if you want to tackle it now.wled00/ota_update.cpp (1)
18-23: Bootloader offsets and ESP32-C5 chip ID verified as correct.The values for ESP32-C5 (chip_id
0x0017, bootloader offset0x2000) and ESP32-C61 (bootloader offset0x0000) match ESP-IDF documentation. Bootloader offsets are already properly hoisted into theBOOTLOADER_OFFSETconstant.Optional: Consider defining chip ID constants. The chip IDs are currently hardcoded in the validation checks (lines 415–450), though they are documented in comments (lines 405–413). Replacing hardcoded values like
0x0017with named constants (e.g.,ESP32C5_CHIP_ID = 0x0017) would improve maintainability and reduce drift risk if ESP-IDF updates these values.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3433 🏁 Script executed: Length of output: 7821
🚨 Breaking Differences Requiring Conditional Code1️⃣ Channel Count (affects max analog LEDs)
Impact: 2️⃣ Timer Bit Width (duty resolution)
Impact: Duty calculations need 3️⃣ Clock Sources (critical API change!)
Breaking: Code using
4️⃣ High-Speed Mode
5️⃣ New Features (C5/C6/P4 only)SOC_LEDC_GAMMA_CURVE_FADE_SUPPORTED (1) // Hardware gamma correction
SOC_LEDC_GAMMA_CURVE_FADE_RANGE_MAX (16) // 16-step gamma curves
SOC_LEDC_FADE_PARAMS_BIT_WIDTH (10) // 10-bit fade params
SOC_LEDC_SUPPORT_SLEEP_RETENTION (1) // Preserve state in sleepOpportunity: Could leverage hardware gamma on C5/C6/P4 for smoother fades. 6️⃣ Fade Stop Support
Impact: 📋 Recommended Conditional Compilation Strategy// In bus_manager.cpp or BusPwm class:
// 1. Channel count
`#if` SOC_LEDC_CHANNEL_NUM == 6
`#define` WLED_MAX_ANALOG_CHANNELS 6 // C3/C5/C6
`#else`
`#define` WLED_MAX_ANALOG_CHANNELS 8 // ESP32/S2/S3/P4
`#endif`
// 2. Clock source selection
`#ifdef` SOC_LEDC_SUPPORT_PLL_DIV_CLOCK
ledc_timer.clk_cfg = LEDC_USE_PLL_DIV_CLK; // C5/C6/P4
`#else`
ledc_timer.clk_cfg = LEDC_USE_APB_CLK; // ESP32/S2/S3/C3
`#endif`
// 3. Speed mode
`#ifdef` SOC_LEDC_SUPPORT_HS_MODE
ledc_channel.speed_mode = LEDC_HIGH_SPEED_MODE; // Only ESP32 classic
`#else`
ledc_channel.speed_mode = LEDC_LOW_SPEED_MODE; // All others
`#endif`
// 4. Duty resolution (use SOC macro directly)
ledc_timer.duty_resolution = SOC_LEDC_TIMER_BIT_WIDTH;
// 5. Fade stop (if using fade API)
`#ifdef` SOC_LEDC_SUPPORT_FADE_STOP
ledc_fade_stop(LEDC_LOW_SPEED_MODE, channel);
`#endif`
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
wled00/dmx_output.cpp (1)
71-76:⚠️ Potential issue | 🟡 MinorFix indentation of the preprocessor block in
initDMXOutput().The
#if/#else/#endiflines are indented by one space; this file requires 2-space indentation.🔧 Proposed fix
void initDMXOutput() { - `#if` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32S2) + `#if` defined(ESP8266) || defined(CONFIG_IDF_TARGET_ESP32C3) || defined(CONFIG_IDF_TARGET_ESP32C5) || defined(CONFIG_IDF_TARGET_ESP32C6) || defined(CONFIG_IDF_TARGET_ESP32C61) || defined(CONFIG_IDF_TARGET_ESP32S2) dmx.init(512); // initialize with bus length - `#else` + `#else` dmx.initWrite(512); // initialize with bus length - `#endif` + `#endif` }As per coding guidelines:
wled00/**/*.cpp: Use 2-space indentation for C++ source files (.cpp).usermods/audioreactive/audio_source.h (1)
25-33:⚠️ Potential issue | 🟡 MinorAlign the warning text with the expanded target guard.
The guard now includes C5/C6/C61/H2, but the warning/error still says only C2/C3, which is confusing for builders on the newly added targets.
💬 Suggested wording update
- `#error` This audio reactive usermod does not support ESP32-C2 or ESP32-C3. + `#error` This audio reactive usermod is not supported on this MCU (C2/C3/C5/C6/C61/H2). ... - `#warning` This audio reactive usermod does not support ESP32-C2 and ESP32-C3. + `#warning` This audio reactive usermod is not supported on this MCU (C2/C3/C5/C6/C61/H2).wled00/FX_fcn.cpp (1)
1194-1211:⚠️ Potential issue | 🟠 MajorFix I2S memory estimation for single‑I2S bus placement.
usesI2Sstill assumes the single‑I2S bus is the first digital bus (digitalCount == 1). The bus allocation strategy now places single‑I2S on the last bus (ESP32 bus 8, ESP32‑S2 bus 4), so the I2S DMA memory estimate can be skipped entirely and undercountmaxI2S.🔧 Example adjustment
-#if defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S3) - const bool usesI2S = ((useParallelI2S && digitalCount <= 8) || (!useParallelI2S && digitalCount == 1)); -#elif defined(CONFIG_IDF_TARGET_ESP32S2) - const bool usesI2S = (useParallelI2S && digitalCount <= 8); +#if defined(CONFIG_IDF_TARGET_ESP32) || defined(CONFIG_IDF_TARGET_ESP32S3) + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || + (!useParallelI2S && digitalCount == 9); +#elif defined(CONFIG_IDF_TARGET_ESP32S2) + const bool usesI2S = (useParallelI2S && digitalCount <= 8) || + (!useParallelI2S && digitalCount == 5);Based on learnings, “single I2S” now uses the last bus (ESP32 bus 8 / S2 bus 4), so memory accounting must key off that bus index instead of
digitalCount == 1.wled00/wled.h (1)
334-344:⚠️ Potential issue | 🔴 CriticalGuard RX/TX usage on boards that don't define those macros.
The current logic silently uses RX=3 and TX=1 for any non-classic ESP32 variant that doesn't define those macros. However, non-classic ESP32 variants (ESP32-C3, ESP32-C5, ESP32-C6, etc.) have different UART0 pins—falling back to 3/1 for those variants is incorrect. For example, ESP32-C3 UART0 defaults to GPIO20/21, not GPIO3/1.
The fix should guard RX/TX usage explicitly and only provide the 3/1 fallback for classic ESP32. Add an error directive for variants without RX/TX defined to prevent silent pin misconfigurations.
🔧 Proposed hardening
-#if defined(ARDUINO_ARCH_ESP32) && (!defined(CONFIG_IDF_TARGET_ESP32) || (defined(RX) && defined(TX))) - // use RX/TX as set by the framework - these boards do _not_ have RX=3 and TX=1 - constexpr uint8_t hardwareRX = RX; - constexpr uint8_t hardwareTX = TX; -#else - // use defaults for RX/TX - constexpr uint8_t hardwareRX = 3; - constexpr uint8_t hardwareTX = 1; -#endif +#if defined(ARDUINO_ARCH_ESP32) && defined(RX) && defined(TX) + // use RX/TX as set by the framework - these boards do _not_ have RX=3 and TX=1 + constexpr uint8_t hardwareRX = RX; + constexpr uint8_t hardwareTX = TX; +#elif defined(CONFIG_IDF_TARGET_ESP32) + // use defaults for RX/TX on classic ESP32 + constexpr uint8_t hardwareRX = 3; + constexpr uint8_t hardwareTX = 1; +#else + `#error` "RX/TX are not defined for this ESP32 variant. Please define RX/TX (or provide a board-specific default)." +#endif
🧹 Nitpick comments (4)
wled00/ws.cpp (1)
86-99: Replace the DDP header magic number and align types.
10is a protocol constant; making it a namedconstexprimproves readability and avoids repeating magic numbers. While you’re here, consider makingoffsetasize_tto avoid mixed signed/unsigned arithmetic.♻️ Suggested tweak
- constexpr int offset = 1; // offset to skip protocol byte + constexpr size_t offset = 1; // offset to skip protocol byte + constexpr size_t DDP_HEADER_LEN = 10; switch (data[0]) { @@ - if (len < size_t(10 + offset)) return; // DDP header is 10 bytes (+1 protocol byte) + if (len < DDP_HEADER_LEN + offset) return; // DDP header is 10 bytes (+1 protocol byte) size_t ddpDataLen = (data[8+offset] << 8) | data[9+offset]; // data length in bytes from DDP header @@ - if (len < size_t(10 + offset + ddpDataLen)) return; // not enough data, prevent out of bounds read + if (len < DDP_HEADER_LEN + offset + ddpDataLen) return; // not enough data, prevent out of bounds readBased on learnings: "In WLED code reviews, when code is modified or added, look for 'magic numbers' (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when meaningful constants exist in the codebase."
wled00/udp.cpp (1)
723-743: Consider usingWLED_BOARDto avoid the growing#elifchain.Now that
wled_boards.hprovides a canonicalWLED_BOARD, this can be simplified and kept in sync automatically.♻️ Possible refactor
-#ifdef ESP8266 - data[38] = NODE_TYPE_ID_ESP8266; -#elif defined(CONFIG_IDF_TARGET_ESP32C5) - data[38] = NODE_TYPE_ID_ESP32C5; -#elif defined(CONFIG_IDF_TARGET_ESP32C6) - data[38] = NODE_TYPE_ID_ESP32C6; -#elif defined(CONFIG_IDF_TARGET_ESP32C61) - data[38] = NODE_TYPE_ID_ESP32C61; -#elif defined(CONFIG_IDF_TARGET_ESP32C3) - data[38] = NODE_TYPE_ID_ESP32C3; -#elif defined(CONFIG_IDF_TARGET_ESP32S3) - data[38] = NODE_TYPE_ID_ESP32S3; -#elif defined(CONFIG_IDF_TARGET_ESP32S2) - data[38] = NODE_TYPE_ID_ESP32S2; -#elif defined(CONFIG_IDF_TARGET_ESP32P4) - data[38] = NODE_TYPE_ID_ESP32P4; -#elif defined(ARDUINO_ARCH_ESP32) - data[38] = NODE_TYPE_ID_ESP32; -#else - data[38] = NODE_TYPE_ID_UNDEFINED; -#endif + data[38] = WLED_BOARD;wled00/FXparticleSystem.cpp (1)
502-510: Update fast-divide comments to match the capability flag.The guard is now macro-driven, but the inline comments still name specific targets. Consider making the comment macro-based to avoid drift.
✏️ Suggested comment tweak (apply to each affected block)
- `#else` // division is faster on ESP32, S2 and S3 + `#else` // division is faster when WLED_HAVE_FAST_int_DIVIDE is setAlso applies to: 516-528, 931-937, 959-969, 1412-1424, 1705-1709, 1726-1732
wled00/wled.cpp (1)
989-996: Consider adding IDF version guard forWiFi.getBand()for consistency.The guard
#if defined(ARDUINO_ARCH_ESP32) && SOC_WIFI_SUPPORT_5Gdiffers from the guards used forWiFi.setBandMode()at lines 537 and 748, which also checkESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 4, 2). While in practiceSOC_WIFI_SUPPORT_5Gcurrently implies IDF 5.4+ (since ESP32-C5 is the only chip defining it), adding the version check would maintain consistency and future-proof the code.♻️ Optional: Add version guard for consistency
- `#if` defined(ARDUINO_ARCH_ESP32) && SOC_WIFI_SUPPORT_5G + `#if` defined(ARDUINO_ARCH_ESP32) && defined(SOC_WIFI_SUPPORT_5G) && (ESP_IDF_VERSION >= ESP_IDF_VERSION_VAL(5, 4, 2)) auto wifiBand = WiFi.getBand(); DEBUG_PRINT(wifiBand == WIFI_BAND_2G ? F(" (2.4GHz)") : (wifiBand == WIFI_BAND_5G ? F(" (5GHz)"): F(" (other)"))); `#else`
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🌐 Web query: 💡 Result: In ESP-IDF v5.5.2, the LEDC (LED PWM) feature flags and limits are exposed via SoC capability macros in:
Common “is LEDC present?” macro
ESP32 (classic) LEDC capability macros (examples)On ESP32,
Newer targets may expose different/additional LEDC macros (example: ESP32-C5)On ESP32-C5, the LEDC capability set includes (among others) [2]:
If you tell me your exact target (esp32 / esp32s2 / esp32s3 / esp32c3 / esp32c6 / esp32h2 / esp32c5, etc.), I can list the exact Sources: ESP-IDF Programming Guide (v5.5.2) SoC capabilities pages for ESP32 and ESP32-C5 [1][2]. 🌐 Web query: 💡 Result: In ESP-IDF commit
Practical impact: any code (especially low-level/LL code) that directly referenced Sources: Citations: 🌐 Web query: 💡 Result: ESP-IDF commit What changed for “LEDC duty” (relevant to ESP32-C5)
Practical impact
Sources: commit page and diff context [1][2], ESP-IDF LEDC docs (latest / ESP32-C5) [3][4]. 🏁 Script executed: Length of output: 3433
📊 LEDC Capabilities Comparison Table
🚨 Critical Breaking Changes for WLED1️⃣ Clock Source API (BREAKING)// ❌ OLD CODE (won't compile on C5/C6/P4):
ledc_timer.clk_cfg = LEDC_USE_APB_CLK;
// ✅ NEW CODE:
`#ifdef` SOC_LEDC_SUPPORT_PLL_DIV_CLOCK
ledc_timer.clk_cfg = LEDC_USE_PLL_DIV_CLK; // C5/C6/P4
`#else`
ledc_timer.clk_cfg = LEDC_USE_APB_CLK; // ESP32/S2/S3/C3
`#endif`2️⃣ Duty Field Access (C5/P4 specific)ESP32-P4 (commit fbdb9413): // ❌ OLD: duty_r.duty
uint32_t duty = LEDC.channel_group[gr].channel[ch].duty_r.duty;
// ✅ NEW on P4: duty_r.duty_r
uint32_t duty = LEDC.channel_group[gr].channel[ch].duty_r.duty_r;ESP32-C5 (commit 33fa42ca):
3️⃣ Channel Count// Use SOC macro instead of hardcoded 8:
`#define` WLED_MAX_ANALOG_CHANNELS SOC_LEDC_CHANNEL_NUM // 6 on C3/C5/C6, 8 on others4️⃣ High-Speed Mode`#ifdef` SOC_LEDC_SUPPORT_HS_MODE
ledc_channel.speed_mode = LEDC_HIGH_SPEED_MODE; // Only ESP32 classic
`#else`
ledc_channel.speed_mode = LEDC_LOW_SPEED_MODE; // All others
`#endif`💡 Recommended Code Pattern for bus_manager.cpp// 1. Clock source selection
`#ifdef` SOC_LEDC_SUPPORT_PLL_DIV_CLOCK
ledc_timer_config_t timer_conf = {
.speed_mode = LEDC_LOW_SPEED_MODE,
.duty_resolution = SOC_LEDC_TIMER_BIT_WIDTH, // Use SOC macro
.timer_num = LEDC_TIMER_0,
.freq_hz = 5000,
.clk_cfg = LEDC_USE_PLL_DIV_CLK // C5/C6/P4
};
`#else`
ledc_timer_config_t timer_conf = {
.speed_mode = LEDC_LOW_SPEED_MODE,
.duty_resolution = SOC_LEDC_TIMER_BIT_WIDTH,
.timer_num = LEDC_TIMER_0,
.freq_hz = 5000,
.clk_cfg = LEDC_USE_APB_CLK // ESP32/S2/S3/C3
};
`#endif`
// 2. Duty calculation (account for C5 inclusive range)
uint32_t max_duty = (1 << SOC_LEDC_TIMER_BIT_WIDTH); // Now inclusive on C5!
uint32_t duty = (brightness * max_duty) / 255;
// 3. Set duty using HAL API (avoid direct struct access on IDF V5)
ledc_set_duty(LEDC_LOW_SPEED_MODE, channel, duty);
ledc_update_duty(LEDC_LOW_SPEED_MODE, channel);
// 4. Optional: leverage hardware gamma on C5/C6/P4
`#ifdef` SOC_LEDC_GAMMA_CURVE_FADE_SUPPORTED
// Could use ledc_set_duty_with_gamma() for smoother fades
`#endif`🎯 Summary of Must-Fix Issues
All of these are compile/runtime blockers that must be addressed for C5/C6/P4 support. The good news: all can be handled with conditional compilation using 🧠 Learnings used |
5 participants
C6 needs the newer ESP-IDF, so have one branch for those changes and keep this one just for the C6 specific things
Building on top of V5
Inspired by C6 Experiments
Help Us with Testing
If you own a C6 or C5 board, we like to hear from your experiences! Basic functions are working on our dev boards, but any problem you find will help us stabilize the C5/C6 versions faster. If you find an error, please create an issue ticket (bug report).
Important: Our V5 Branch is Experimental (early stage of development)
You can find a summary of currently open ends here (open ends for C5, C6), here (modernization opportunities) and here (generic problems with V5).
Using Development Tools (VSCode) - recommended
You can build your own C5 or C6 firmware using our standard development environment (VSCode+platformio) on your own computer. Use Platformio "upload and monitor" to write firmware to your board.
Experimental Binaries
In case you are not feeling comfortable using development tools, you can find some firmware binaries that are automatically created by GitHub after each commit.
WLED CI / wled_build / Build Environments (esp32c6dev_4MB) (push)Help Us with Coding
Helpful information and migration guidance
coding guides
We should make sure that the "V5" branch will still compile for "V4" as a backup solution.
When adding code that only works in the new framework, it should be conditionally compiled, like in the examples below
related
Summary by CodeRabbit
New Features
Bug Fixes / Compatibility
Chores